feat(async-flow): add more context to vow rejection #10359

michaelfig · 2024-10-29T01:51:54Z

Description

When an asyncFlow guest calls the host through its replay membrane, keep an ephemeral weakMapStore from any result hostVow to the target, method, (and eventually, whether it is an "eventual" result).

If that hostVow is rejected with an error, annotate the error as described in #10147.

Security Considerations

Error annotations are only revealed to the base causal console, which cannot be captured in vat code (unlike the consoleRecorder shim that is set up during tests in this PR).

Scaling Considerations

No significant increase yet.

Documentation Considerations

n/a

Testing Considerations

n/a

Upgrade Considerations

n/a

cloudflare-workers-and-pages · 2024-10-29T01:53:54Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`733042d`
Status:	✅ Deploy successful!
Preview URL:	https://61d5d4cf.agoric-sdk.pages.dev
Branch Preview URL:	https://mfig-10147-flow-rejects.agoric-sdk.pages.dev

View logs

packages/async-flow/src/template.js

erights · 2024-11-13T18:53:58Z

packages/async-flow/src/template.js

+export const makeTemplateStringsArray = strings =>
+  harden(assign([...strings], { raw: strings }));


Clever, but the attributes matter. The raw should be non-enumerable. How about

Suggested change

export const makeTemplateStringsArray = strings =>

harden(assign([...strings], { raw: strings }));

export const makeTemplateStringsArray = strings => {

const result = [...strings];

defineProperty(result, 'raw', {

value: strings, // Note: same rather than raw version

enumerable: false, // not needed, but good to be explicit

});

return harden(result);

};

I finessed your suggestion a little bit to avoid needing to coerce result with a typecast.

packages/async-flow/src/template.js

erights · 2024-11-13T18:59:02Z

packages/async-flow/src/types.ts

@@ -22,6 +22,12 @@ export type FlowState =
 export type Guest<T extends unknown = any> = T;
 export type Host<T extends Passable = Passable> = T;

+export type HostCall = {


Just from the name, I expected a HostCall to also include the arguments.

I'll explain why this is not the case in a comment (basically, the replay membrane doesn't need them to produce good-enough errors).

…ects

erights · 2024-11-13T22:07:53Z

packages/async-flow/src/template.js

@@ -11,10 +11,21 @@ const BEST_GUESS_ID_REGEX = /^[a-zA-Z_$][a-zA-Z0-9_$]*$/;
 * Return an object that mimics a template strings array.
 *
 * @param {string[]} strings
+ * @param {string[]} [raw] optional raw strings, defaults to `strings`


erights · 2024-11-13T22:09:02Z

packages/async-flow/src/template.js

+  // Make the raw property non-enumerable.
+  defineProperty(result, 'raw', {
+    value: raw,
+    enumerable: false, // not needed, but good to be explicit


With the "finessed" change, the explicit enumerable: false, is needed.

Suggested change

enumerable: false, // not needed, but good to be explicit

enumerable: false,

mhofman · 2024-11-19T13:47:15Z

keep a durable weakMapStore from any result hostVow to the target, method, (and eventually, whether it is an "eventual" result).

I have not reviewed yet, but I'm surprised we need a durable store for this. Since we always replay, why can't we use an ephemeral store?

michaelfig · 2024-11-19T15:33:27Z

keep a durable weakMapStore [...]

I have not reviewed yet, but I'm surprised we need a durable store for this. Since we always replay, why can't we use an ephemeral store?

Ah, good catch. I'll try with an ephemeral store and ensure it still passes tests.

mhofman

I still need to parse the test for this, but here are my preliminary comments on the implementation.

mhofman · 2024-11-19T13:53:29Z

packages/async-flow/test/console-recorder-shim.js

+      if (typeof v === 'function') {
+        return {
+          [p](...args) {
+            if (recording) recording.push({ thisArg: target, method: p, args });


Isn't thisArg a little misleading as a name if it records the proxy's target.

mhofman · 2024-11-19T14:04:21Z

packages/async-flow/test/console-recorder-shim.js

+    apply(target, thisArg, args) {
+      if (recording) recording.push({ thisArg, func: target, args });
+      return Reflect.apply(target, thisArg, args);
+    },


Do we care about this? Do we ever have a callable console?

mhofman · 2024-11-19T14:12:02Z

packages/async-flow/test/replay-membrane.test.js

Nit: the tag config additions are in commit before makeReplayMembrane adds support / documentation for that config.

mhofman · 2024-11-19T14:13:25Z

packages/async-flow/test/async-flow-debug.test.js

+    args,
+  });
+
+  t.true(recording.every(entry => entry.thisArg === originalConsole));


I think by construction of the recording console, this can only be true, right?

mhofman · 2024-11-19T14:22:48Z

packages/async-flow/test/async-flow-debug.test.js

Nit: this file seem to be testing behavior introduced in a later commit.

mhofman · 2024-11-19T15:54:39Z

packages/async-flow/src/replay-membrane.js

+      initHostCall(vow, {
+        target: hostTarget,
+        method: optVerb,
+        eventual: true,
+      });


Oh yeah with eventual send it's gonna be more difficult to record on the guest side since we don't see the guest anymore.

mhofman · 2024-11-19T15:59:46Z

packages/async-flow/src/replay-membrane.js

+      );
+      if (hostVowToCall) {
+        const hostVowCap = toPassableCap(hostVow);
+        if (hostVowToCall.has(hostVowCap)) {


In what case would the map not have the cap?

mhofman · 2024-11-19T16:00:08Z

packages/async-flow/src/replay-membrane.js

+      if (hostVowToCall) {
+        const hostVowCap = toPassableCap(hostVow);
+        if (hostVowToCall.has(hostVowCap)) {
+          const hostCall = hostVowToCall.get(hostVowCap);


Is there a reason to leave the host call in the map after rejection? I think doFulfill should remove from the map as well.

mhofman · 2024-11-19T16:02:49Z

packages/async-flow/src/async-flow.js

@@ -118,6 +118,7 @@ export const prepareAsyncFlowTools = (outerZone, outerOptions = {}) => {
          bijection, // membrane's guest-host mapping
          outcomeKit: makeVowKit(), // outcome of activation as host vow
          isDone: false, // persistently done
+          hostVowToCall: zone.detached().weakMapStore('hostVowToCall'),


Besides changing to an ephemeral store, I think we shouldn't burden ourselves with weak collections and their gc costs since we have delimited lifetimes for these host call entries, and don't need to retain them past settlement. Since the replay membrane is keeping the result vow/promises live anyway, there is no reason to assume those might be dropped earlier either.

mhofman · 2024-11-19T16:06:34Z

packages/async-flow/src/template.js

+ * @param  {any[]} args
+ * @returns {[TemplateStringsArray, ...any[]]}
+ */
+export const inlineTemplateArgs = (tmpl, ...args) => {


I am wondering if the ergonomics wouldn't be simpler with a wrapper for the original template tag function:

inlineArgs(X)`foo bar ${inlinedArg}`;

mhofman

It was a little hard to digest the test. Wondering how we can simplify its reading.

mhofman · 2024-11-19T16:52:15Z

packages/async-flow/test/async-flow-debug.test.js

+  const consStack = i => {
+    const re = indexToStackRegex[i];
+    delete indexToStackRegex[i];
+    t.truthy(re, `recorded entry ${i} has a stack regex`);


Why not accept the re as an argument instead of this definition indirection?

Suggested change

const consStack = i => {

const re = indexToStackRegex[i];

delete indexToStackRegex[i];

t.truthy(re, `recorded entry ${i} has a stack regex`);

const consStack = (i, re) => {

t.truthy(re, `recorded entry ${i} has a stack regex`);

mhofman · 2024-11-19T16:55:19Z

packages/async-flow/test/async-flow-debug.test.js

+  const error = (...args) => cons('error', ...args);
+  const group = (...args) => cons('group', ...args);
+
+  const expectedRecording = {


It's a little weird to see assertions being made during the declaration of the expected value, but then I'm not sure how to do it any better. I also think that it'd be better to run the main "deepEqual" assertion before any nested assertions for specific entries.

I'm wondering if something like the helpers returning a tuple of "method + args" and an optional thunk to run extra assertions. The "method +args" would be used to build an expected result that can be used first with deepEqual. The thunks if present would then be invoked one by one on each recorded entry. The thunk could have a signature of (recordedArgs, i) so that the helper definitions don't need to close over the recording input.

mhofman · 2024-11-19T17:02:25Z

packages/async-flow/test/async-flow-debug.test.js

+test.serial('test host vow rejection logging', async t => {
+  annihilate();
+  const zone1 = makeDurableZone(getBaggage(), 'durableRoot');
+  await testRejectionPlay(t, zone1);
+});


I think I'd like to see this test also written with a restart where the vow is only resolved in the new incarnation.

mhofman · 2024-11-19T17:07:22Z

packages/async-flow/test/async-flow-debug.test.js

+    async guestMethod(gOrch7) {
+      const p2 = gOrch7.vow();
+
+      await eventLoopIteration();


why do we need this new crank?

mhofman · 2024-11-19T17:11:28Z

packages/async-flow/test/async-flow-debug.test.js

+      setImmediate(() => {
+        gOrch7.reject(Error('should be seen'));
+      });
+
+      await eventLoopIteration();


while semantically equivalent to an await eventLoopIteration() followed by a call to reject, I assume this is written like this to put the rejection on an empty stack even if the engine has async stack traces? Can we add a comment to this effect?

mhofman · 2024-11-19T17:13:42Z

packages/async-flow/test/async-flow-debug.test.js

+
+      await eventLoopIteration();
+
+      const caughtP = p2.catch(e => {


I would like to sanity check that the promise doesn't fulfill.

Suggested change

const caughtP = p2.catch(e => {

const caughtP = p2.then(

v => t.fail(`vow fulfilled with value ${v}`),

e => {

mhofman · 2024-11-19T17:20:57Z

packages/async-flow/test/async-flow-debug.test.js

+    3: error('Error#1 ERROR_NOTE:', 'from host error', '(Error#2)'),
+    4: error(
+      'Error#1 ERROR_NOTE:',
+      'from flow',


would it read better as?

Suggested change

'from flow',

'during execution of flow',

mhofman · 2024-11-19T17:34:56Z

packages/async-flow/test/async-flow-debug.test.js

+    [recLen17 - 6]: error('Error#4:', 'should be seen'),
+    [recLen17 - 5]: consStack(recLen17 - 5),
+    [recLen17 - 4]: cons('groupEnd'),
+    [recLen17 - 3]: error('Error#3:', 'this stack'),


Would it make sense to have a message a little more descriptive? maybe:

Suggested change

[recLen17 - 3]: error('Error#3:', 'this stack'),

[recLen17 - 3]: error('Error#3:', 'AsyncFlow1 definition site'),

michaelfig force-pushed the mfig-10147-flow-rejects branch from ffdb74c to cc5a1fb Compare October 29, 2024 01:53

michaelfig force-pushed the mfig-10147-flow-rejects branch 2 times, most recently from 0740c55 to fc1f17a Compare November 1, 2024 02:02

test(async-flow): record console output to test devx

fc58f8d

michaelfig force-pushed the mfig-10147-flow-rejects branch 2 times, most recently from a66a9ce to 49b4c9f Compare November 13, 2024 16:24

feat(async-flow): add more context to vow rejection

588d227

michaelfig force-pushed the mfig-10147-flow-rejects branch from 49b4c9f to 588d227 Compare November 13, 2024 17:00

michaelfig marked this pull request as ready for review November 13, 2024 17:55

michaelfig requested a review from a team as a code owner November 13, 2024 17:55

michaelfig requested review from AgoricTriage, mhofman and erights November 13, 2024 17:55

michaelfig self-assigned this Nov 13, 2024

michaelfig added the asyncFlow related to membrane-based replay and upgrade of async functions label Nov 13, 2024

erights reviewed Nov 13, 2024

View reviewed changes

packages/async-flow/src/template.js Show resolved Hide resolved

erights reviewed Nov 13, 2024

View reviewed changes

packages/async-flow/src/template.js Outdated Show resolved Hide resolved

erights reviewed Nov 13, 2024

View reviewed changes

erights self-requested a review November 13, 2024 19:34

michaelfig added 2 commits November 13, 2024 13:47

fixup! feat(async-flow): add more context to vow rejection

5bd083a

Merge remote-tracking branch 'origin/master' into mfig-10147-flow-rej…

1d9475a

…ects

erights reviewed Nov 13, 2024

View reviewed changes

erights self-requested a review November 13, 2024 22:12

fixup! feat(async-flow): add more context to vow rejection

733042d

mhofman reviewed Nov 19, 2024

View reviewed changes

mhofman self-requested a review November 19, 2024 16:09

mhofman reviewed Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(async-flow): add more context to vow rejection #10359

feat(async-flow): add more context to vow rejection #10359

michaelfig commented Oct 29, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Oct 29, 2024 •

edited

Loading

erights Nov 13, 2024 •

edited

Loading

michaelfig Nov 13, 2024

erights Nov 13, 2024

michaelfig Nov 13, 2024

erights Nov 13, 2024

erights Nov 13, 2024 •

edited

Loading

mhofman commented Nov 19, 2024

michaelfig commented Nov 19, 2024

mhofman left a comment

mhofman Nov 19, 2024

mhofman Nov 19, 2024

mhofman Nov 19, 2024

mhofman Nov 19, 2024

mhofman Nov 19, 2024

mhofman Nov 19, 2024

mhofman Nov 19, 2024

mhofman Nov 19, 2024

mhofman Nov 19, 2024 •

edited

Loading

mhofman Nov 19, 2024

mhofman left a comment

mhofman Nov 19, 2024

mhofman Nov 19, 2024

mhofman Nov 19, 2024

mhofman Nov 19, 2024

mhofman Nov 19, 2024

mhofman Nov 19, 2024

mhofman Nov 19, 2024

mhofman Nov 19, 2024

		export const makeTemplateStringsArray = strings =>
		harden(assign([...strings], { raw: strings }));

	enumerable: false, // not needed, but good to be explicit
	enumerable: false,

-      const caughtP = p2.catch(e => {
+      const caughtP = p2.then(
+        v => t.fail(`vow fulfilled with value ${v}`),
+        e => {

	[recLen17 - 3]: error('Error#3:', 'this stack'),
	[recLen17 - 3]: error('Error#3:', 'AsyncFlow1 definition site'),

feat(async-flow): add more context to vow rejection #10359

Are you sure you want to change the base?

feat(async-flow): add more context to vow rejection #10359

Conversation

michaelfig commented Oct 29, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Oct 29, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

erights Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erights Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

mhofman commented Nov 19, 2024

michaelfig commented Nov 19, 2024

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelfig commented Oct 29, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Oct 29, 2024 •

edited

Loading

erights Nov 13, 2024 •

edited

Loading

erights Nov 13, 2024 •

edited

Loading

mhofman Nov 19, 2024 •

edited

Loading